Skip to content

Fix underflow when trading - #1969

Merged
Flamefire merged 6 commits into
Return-To-The-Roots:masterfrom
ottml:fix_trading_overflow
Aug 26, 2026
Merged

Fix underflow when trading#1969
Flamefire merged 6 commits into
Return-To-The-Roots:masterfrom
ottml:fix_trading_overflow

Conversation

@ottml

@ottml ottml commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Steps to reproduce:

  • Build two storehouses
  • Both storehouses have figures with one type, for example woodcutter
  • Both storehouses have 2 woodcutters
  • Send 1 woodcutter to ally storehouse
  • Then 3 woodcutters are sent to the ally instead of 1

It sends the requested number from the first WH and then basically ALL from the other warehouses due to count - available underflowing

@ottml

ottml commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

@Flamefire @Flow86 Could it be that some permissions are missing for pullrequests from a fork? Because the ci above is not starting because of

2 workflows awaiting approval

@ottml
ottml force-pushed the fix_trading_overflow branch from a9c088c to 2b296a3 Compare August 17, 2026 21:04

@Flamefire Flamefire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steps to reproduce:

  • Build two storehouses
    
  • Both storehouses have figures with one type, for example woodcutter
    
  • Both storehouses have 2 woodcutters
    
  • Send 1 woodcutter to ally storehouse
    
  • Then 3 woodcutters are sent to the ally instead of 1
    

I don't see how 2 WHs are required for this bug. It seems the issue is that all woodcutters are sent instead of only the requested one. Can you explain?

@Flamefire @Flow86 Could it be that some permissions are missing for pullrequests from a fork? Because the ci above is not starting because of

2 workflows awaiting approval

That's intentional: Especially with AI PRs this avoids people flooding CI. Not sure if (still) required here though

Comment thread libs/s25main/buildings/nobBaseWarehouse.cpp
Comment thread tests/s25Main/integration/testTrading.cpp Outdated
Comment thread tests/s25Main/integration/testTrading.cpp Outdated
@ottml

ottml commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Steps to reproduce:

  • Build two storehouses
    
  • Both storehouses have figures with one type, for example woodcutter
    
  • Both storehouses have 2 woodcutters
    
  • Send 1 woodcutter to ally storehouse
    
  • Then 3 woodcutters are sent to the ally instead of 1
    

I don't see how 2 WHs are required for this bug. It seems the issue is that all woodcutters are sent instead of only the requested one. Can you explain?

@Flamefire @Flow86 Could it be that some permissions are missing for pullrequests from a fork? Because the ci above is not starting because of

2 workflows awaiting approval

That's intentional: Especially with AI PRs this avoids people flooding CI. Not sure if (still) required here though

If you have only one warehouse we get a overflow but this has now effect because the warehouse loop is left after the one woodcutter is sent from this warehouse. Only if there are more then one warehouse with the specific job the overflow has an effect. Because the count variable has an infite value and the count==0 check does never match.

Ah ok this is because of the AI. Must the approval be done after each commit or only for a newly created pull request? If this must be approved every time it could be difficult to fix unit-test and clang-tidy.

Comment thread tests/s25Main/integration/testTrading.cpp Outdated
Comment thread tests/s25Main/integration/testTrading.cpp Outdated
@Flamefire

Copy link
Copy Markdown
Member

If you have only one warehouse we get a overflow but this has now effect because the warehouse loop is left after the one woodcutter is sent from this warehouse. Only if there are more then one warehouse with the specific job the overflow has an effect. Because the count variable has an infite value and the count==0 check does never match.

Ok so the issue is that it sends the requested number from the first WH and then basically ALL from the other warehouses due to count - available underflowing. Can you add that to the PR description?

Ah ok this is because of the AI. Must the approval be done after each commit or only for a newly created pull request? If this must be approved every time it could be difficult to fix unit-test and clang-tidy.

It's done for all PRs just because they could be AI.
Not fully sure. At least reviewers get a notification on pushes and can re-approve timely

Comment thread tests/s25Main/integration/testTrading.cpp Outdated
Comment thread tests/s25Main/integration/testTrading.cpp Outdated
@Flamefire Flamefire changed the title Fix trading overflow Fix underflow when trading Aug 26, 2026
@Flamefire
Flamefire force-pushed the fix_trading_overflow branch from ddc7684 to b484911 Compare August 26, 2026 17:57
ottml and others added 6 commits August 26, 2026 20:09
@Flamefire
Flamefire force-pushed the fix_trading_overflow branch from b484911 to fac0308 Compare August 26, 2026 18:10
@Flamefire
Flamefire enabled auto-merge August 26, 2026 18:11
@Flamefire
Flamefire merged commit de53cb1 into Return-To-The-Roots:master Aug 26, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants